Fix error occurring in `useEffect` on `hmr` #308

PatrykWalach · 2025-01-06T23:03:13Z

Rebase of #264

rbalicki2

lgtm, just rewrote the comment slightly for grammar and clarity

rbalicki2

Sorry, I forgot to do a thorough review before accepting 😂 Do we also need this in useUpdatableDisposableState?

If so, there are probably enough places where we do this that it makes sense to have a useEffectIgnoringHmrAndStrictMode hook.

It might return the lastCommittedParentCache, in which case we don't need to do anything extra in useCachedResponsivePreCommitValue

rbalicki2 · 2025-01-07T16:29:17Z

libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts

@@ -51,6 +51,14 @@ export function useCachedResponsivePrecommitValue<T>(
  const lastCommittedParentCache = useRef<ParentCache<T> | null>(null);

  useEffect(() => {
+    // react reruns all `useEffect` in HMR since it doesn't know if the


// Since React doesn't know if the code inside of a useEffect has changed, // it unmounts and remounts all useEffect calls when doing HMR. This breaks // this hook's assumptions! The `onCommit` is called twice. // // Since this is a library, the user can't change this code, so we are safe // to skip this rerun. // // This also prevents the useEffect from running twice in Strict Mode.

rbalicki2 · 2025-01-07T16:31:32Z

libs/isograph-react-disposable-state/src/useLazyDisposableState.ts

  useEffect(() => {
+    // react reruns all `useEffect` in HMR since it doesn't know if the


// Since React doesn't know if the code inside of a useEffect has changed, // it unmounts and remounts all useEffect calls when doing HMR. This breaks // this hook's assumptions! The cleanupFn is called unexpectedly during HMR, // and the item is not recreated. (Even recreating the item would be a bad outcome, // as that would reissue network requests.) // // Since this is a library, the user can't change this code, so we are safe // to skip this rerun. // // This also prevents the useEffect from running twice in Strict Mode.

rbalicki2 · 2025-01-07T16:36:43Z

libs/isograph-react-disposable-state/src/useDisposableState.ts

+
+  useEffect(
+    function cleanupItemCleanupPairRefIfSetStateNotCalled() {
+      if (lastCommittedParentCache.current === parentCache) {


// Since React doesn't know if the code inside of a useEffect has changed, // it unmounts and remounts all useEffect calls when doing HMR. This breaks // this hook's assumptions! The item in the `itemCleanupPairRef` will be disposed. // // Since this is a library, the user can't change this code, so we are safe // to skip this rerun. // // This also prevents the useEffect from running twice in Strict Mode.

Do we also need to have this check above, in the other useEffect?

The second one will not throw because it sets the ref to null. However, because of this, we will lose the current state, which is not perfect.

yeah, seems better to not set it to null!

Oh wait, this should be good, because it will cleanup only when we set with a function and when the ref is still not cleaned up, hmr shouldn't be able to trigger it twice

wdym? Which useEffect are you referring to?

useEffect( function cleanupItemCleanupPairRefAfterSetState() { if (stateFromDisposableStateHook !== UNASSIGNED_STATE) { if (itemCleanupPairRef.current !== null) { itemCleanupPairRef.current[1](); itemCleanupPairRef.current = null; } else { throw new Error( 'itemCleanupPairRef.current is unexpectedly null. ' + 'This indicates a bug in react-disposable-state.', ); } } }, [stateFromDisposableStateHook], );

This one should cleanup only when these two conditions are met, reruning this on hmr has no effect.

I posted this stuff way too quickly. This will throw on hmr but I think just removing the throw should be enough

…rykWalach/isograph into fix-use-effect-error-on-hmr-rebase-2

PatrykWalach · 2025-01-11T13:07:01Z

The solution used here is not good

on the first render useEffect will execute (commit and return cleanup)
on hmr it will cleanup, then the guard will prevent another execute (commit and return cleanup)

so we end up in a cleaned-up state

The reason it doesn't cause any issues now is probably because of gcBuffer.

rbalicki2 · 2025-01-11T13:19:07Z

Ah yeah, you're totally right. Good call re: gcBuffer. I'm going to think about this. I wonder if we need to try to detect HMR/double mounting more directly.

PatrykWalach · 2025-01-13T19:16:41Z

on useEffect cleanup we will set itemCleanupPairRef to null, if the component unmounted this has no effect, if it was hmr then cache will once again go through temporaryRetain cycle.

This is not tested. It might be easier to test after merging the second pull request.

…fect-error-on-hmr-rebase-2' of github.com:PatrykWalach/isograph into fix-use-effect-error-on-hmr-rebase-2

PatrykWalach · 2025-01-13T20:37:11Z

Now the cycle is

commit -> cache is populated
cleanup -> set maybeHiddenOrFastRefresh to true, clean up cache
if component is unmounted, that's the end, if it's hmr:
commit -> read maybeHiddenOrFastRefresh and skip cacheItem.permanentRetainIfNotDisposed so there's no error, repopulate cache

so at the end we are in the populated state.

we don't need to set itemCleanupPairRef to null because cleanup runs just before cache is populated which will override itemCleanupPairRef and there are no renders in between.

useUpdatableDisposableState still needs to be fixed and it can't be fixed in the same way, as there's no way to repopulate 🤔

PatrykWalach added 5 commits January 7, 2025 00:03

fix error with hot module replacement

441b1df

update tests

10ce0e2

fix cleanup

61b16b9

don't use stale itemCleanupPairRef

4fbfb36

add comment for guards added in useEffects

e9d1845

PatrykWalach force-pushed the fix-use-effect-error-on-hmr-rebase-2 branch from 9b897fd to e9d1845 Compare January 6, 2025 23:03

rbalicki2 approved these changes Jan 7, 2025

View reviewed changes

rbalicki2 requested changes Jan 7, 2025

View reviewed changes

PatrykWalach added 4 commits January 10, 2025 22:00

Merge branch 'main' into fix-use-effect-error-on-hmr-rebase-2

ff7cd3b

fix throwing in useDisposableState

52bef46

fix double cleanup in useUpdatableDisposableState

820a8f7

Merge branch 'fix-use-effect-error-on-hmr-rebase-2' of github.com:Pat…

23fb36a

…rykWalach/isograph into fix-use-effect-error-on-hmr-rebase-2

detect hmr

20c773e

PatrykWalach added 14 commits January 13, 2025 20:57

fix errors

78c6037

set itemCleanupPairRef to null on cleanup

2e24a7f

revert last

3f441e0

use ref of type bool to detect hmr

d3de5d4

fix error with hot module replacement

ef925c6

update tests

a6af861

fix cleanup

b377add

don't use stale itemCleanupPairRef

faf696a

add comment for guards added in useEffects

8352c72

fix throwing in useDisposableState

c0083e3

fix double cleanup in useUpdatableDisposableState

bef4639

detect hmr

652be84

fix errors

8b0fdab

set itemCleanupPairRef to null on cleanup

e329219

PatrykWalach added 2 commits January 13, 2025 21:20

revert last

ad22a27

use ref of type bool to detect hmr

50f95ea

PatrykWalach force-pushed the fix-use-effect-error-on-hmr-rebase-2 branch from d3de5d4 to 50f95ea Compare January 13, 2025 20:20

Merge branches 'fix-use-effect-error-on-hmr-rebase-2' and 'fix-use-ef…

402ac00

…fect-error-on-hmr-rebase-2' of github.com:PatrykWalach/isograph into fix-use-effect-error-on-hmr-rebase-2

Merge branch 'main' into fix-use-effect-error-on-hmr-rebase-2

420d291

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error occurring in `useEffect` on `hmr` #308

Fix error occurring in `useEffect` on `hmr` #308

PatrykWalach commented Jan 6, 2025

rbalicki2 left a comment

rbalicki2 left a comment

rbalicki2 Jan 7, 2025

rbalicki2 Jan 7, 2025

rbalicki2 Jan 7, 2025

PatrykWalach Jan 7, 2025

rbalicki2 Jan 7, 2025

PatrykWalach Jan 7, 2025

rbalicki2 Jan 7, 2025

PatrykWalach Jan 7, 2025

PatrykWalach Jan 7, 2025

PatrykWalach commented Jan 11, 2025 •

edited

Loading

rbalicki2 commented Jan 11, 2025

PatrykWalach commented Jan 13, 2025 •

edited

Loading

PatrykWalach commented Jan 13, 2025 •

edited

Loading

		useEffect(() => {
		// react reruns all `useEffect` in HMR since it doesn't know if the

Fix error occurring in useEffect on hmr #308

Are you sure you want to change the base?

Fix error occurring in useEffect on hmr #308

Conversation

PatrykWalach commented Jan 6, 2025

rbalicki2 left a comment

Choose a reason for hiding this comment

rbalicki2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatrykWalach commented Jan 11, 2025 • edited Loading

rbalicki2 commented Jan 11, 2025

PatrykWalach commented Jan 13, 2025 • edited Loading

PatrykWalach commented Jan 13, 2025 • edited Loading

Fix error occurring in `useEffect` on `hmr` #308

Fix error occurring in `useEffect` on `hmr` #308

PatrykWalach commented Jan 11, 2025 •

edited

Loading

PatrykWalach commented Jan 13, 2025 •

edited

Loading

PatrykWalach commented Jan 13, 2025 •

edited

Loading